Skip to content

feat(migrations): require constant defaults, resolve complex ones in the body - #2784

Open
krlmlr wants to merge 7 commits into
mainfrom
claude/migrations-const-defaults
Open

feat(migrations): require constant defaults, resolve complex ones in the body#2784
krlmlr wants to merge 7 commits into
mainfrom
claude/migrations-const-defaults

Conversation

@krlmlr

@krlmlr krlmlr commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Reworked per review direction: no grandfather mechanism.
Two commits:

1. refactor: move the 23 non-constant defaults into the bodies

All non-constant defaults across the 22 migrated functions on main
become constant NULL in the signature,
resolved in the body once all arguments are available:

  • V(graph) / E(graph) selectors (19 arguments):
    alpha_centrality(), betweenness(), closeness(), diversity(),
    edge_betweenness(), harmonic_centrality(), page_rank(),
    power_centrality(), strength(), all_simple_paths(),
    all_shortest_paths(), constraint(), degree(), distances()
    (v and to), ego(), ego_size(), knn(), make_ego_graph(),
    shortest_paths(), which_mutual();
  • as_adjacency_matrix(sparse = igraph_opt("sparsematrices"));
  • max_bipartite_match(eps = .Machine$double.eps).

Body pattern, inserted right after the ARG_HANDLE block
(after ensure_igraph() where present):

if (is.null(vids)) {
  vids <- V(graph)
}

Behavior notes for review:

  • Passing the selector NULL explicitly now means the full vertex/edge
    set, same as omitting the argument.
    Previously an explicit NULL (including a computed c() that came out
    empty) was coerced to an empty selection — an undocumented, untested
    accident of as_igraph_vs(). The new semantics are pinned in
    test-constant-defaults.R; if you'd rather keep the accidental
    empty-selection meaning, say so and I'll switch the resolution to a
    missing() check instead.
  • Watch the interplay with Standardize the handling of optional vertex parameters #1572, which settled on NULL meaning
    "no vertex" for optional single-vertex parameters
    (random_spanning_tree(vid = NULL)). Plural selectors saying
    "NULL = all" vs. singular saying "NULL = none" is a distinction
    worth an explicit line in the docs policy.
  • This fixes a live bug on main: legacy positional recovery of
    diversity() died with a spurious "supplied more than once" error
    because re-evaluating its V(graph) default never compares
    identical(). With a constant default the recovery machinery needs no
    further changes — the defect class fix(migrations): detect supplied tail args with missing() #2783 addressed disappears at the
    root, per "avoid nonconst defaults at all".

2. feat: the generator enforces the rule — no escape hatch

is_constant_default() (recursive AST check): constant = literals,
NULL/TRUE/FALSE/NA*/Inf/NaN, c()/list() of constants,
typed empty vectors (integer(), numeric(), ...),
unary sign, parentheses, the deprecated() sentinel.
Everything else errors at generation time —
option lookups, V(graph), cross-references (bins * 2), RNG draws,
rep(), .Machine$…, bare symbols including T/F.
A leftover nonconst_defaults field errors with its own message.
Documented in tools/migrations/README.md
(and the matching rule prose is on #2757).

Round 2: the whole package

After merging main (which migrated the games/layout/make/… topics
behind the ellipsis), the same treatment now covers every exported,
non-deprecated, hand-written function
— the remaining 106 non-constant
defaults across 67 functions, migrated or not, plus the two
plot_dendrogram() S3 methods for coherence with their generic.
The registry new literals of the 32 affected migrated functions carry
the same NULL defaults, so the regenerated recovery blocks stay
constant.

File Functions Defaults What
games.R 15 29 type.dist, pref.matrix, types, pref, agebins, p — cross-references, incl. the sample_*/spec-wrapper pairs
layout.R 11 26 vcount()/edge_density()-derived constants, drl seed + options, normalize() limits
attributes.R 9 9 index = V(graph)/E(graph) accessors, getters and setters
print.R 1 6 print.igraph()'s six igraph_opt()-backed arguments
structural-properties.R 6 6 bfs()/dfs() rho = parent.frame(), eids/v selectors
make.R 3 4 make_graph() n/dir and the directed/undirected wrappers (missing()is.null())
embedding.R 2 3 cvec (references weights), options = arpack_defaults()
operators.R 3 3 compose()/disjoint_union() graph.attr.comb, reverse_edges(eids = )
plot.R 1 3 mark.col/mark.border (resolve after the mark.groups coercion), mark.shape = 1/20.5
cocitation.R, community.R, plot.shapes.R 5 6 v selectors, contract()/plot_dendrogram() options, add_shape() clip/plot
11 more files 11 11 one each: hits_scores(), plot_hierarchy(), as_undirected(), local_efficiency(), graphlet_proj(), sample_motifs(), indent_print(), similarity(), simplify(), stochastic_matrix(), count_triangles()

Special cases, handled deliberately: cross-referencing defaults resolve
in signature order (area before repulserad/cellsize, types
before pref, agebins before pref); layout_with_drl()'s random
default seed is now drawn inside the function with unchanged RNG stream
consumption for defaulted calls; options = arpack_defaults() resolves
in the body; layout_with_gem(temp.min = 1/10) and
plot.igraph(mark.shape = 1/2) became the literals 0.1/0.5 instead
of NULL; sample_motifs(cut.prob = NULL) needs no resolution because
the C layer already treats NULL as "no cuts" (matching motifs());
and two places keep NULL as a legal value and therefore resolve via
missing() instead of is.null(): print.igraph(max.lines = NULL)
still prints everything, and normalize(ymin = NULL) still disables
normalization along that axis (as in norm_coords()). The
explicit-NULL flip described above now also applies to the
option-backed combination specs (edge.attr.comb = NULL etc. used to
mean an empty combination spec, dropping attributes).

c() defaults were NULL in disguise

c() evaluates to NULL. The five empty-HRG fields that fit_hrg()
builds when no starting model is given (formerly hrg()'s signature
defaults left = c(), right = c(), …) therefore declared NULL values
that historically meant "empty sequence". Under the new doctrine NULL
is the resolve-in-body sentinel, so NULL-as-empty and NULL-as-default
cannot coexist: empty-sequence defaults must be spelled as typed
empty vectors
(numeric() here — the fields are as.numeric()-coerced
on the spot; layout_as_tree()'s root = numeric() was already
spelled right and the checker now accepts it as a literal). Tests pass
integer() where an empty id selection is meant
(max_degree(g, v = integer()) is 0). Passing NULL explicitly to
mean "empty" is no longer supported for treated arguments — a
deliberate breaking change
: max_degree(g, v = NULL) now means all
vertices, pinned in test-constant-defaults.R.

Remaining worklist (not this PR)

The exported hand-written remainder is zero: scanning every
function definition against the classifier finds no non-constant
default outside R/aaa-*.R. What is left is the Stimulus-generated
_impl layer — 103 defaults across 78 internal wrappers — and handling
those in the generator (constant-ness check there, or dropping defaults
from _impl signatures entirely) is still under discussion.

Validation: generator idempotent (generate → air → generate is a
byte-identical fixed point); full suite FAIL 0 · PASS 9250 (the only
error is the known sandbox network fetch in test-foreign.R);
gate-on IGRAPH_LIFECYCLE_ERRORS=true migration/constant-defaults
filter FAIL 0 · PASS 151; 85 Rd files regenerated via
devtools::document().

claude added 2 commits July 26, 2026 21:35
… body

The 23 non-constant defaults across the 22 migrated functions become
constant: `V(graph)`/`E(graph)` selectors, as_adjacency_matrix()'s
`igraph_opt("sparsematrices")`, and max_bipartite_match()'s
`.Machine$double.eps` are now declared as `NULL` in the signature and
resolved in the body once all arguments are available.

Behavior notes:

- Passing the selector NULL explicitly now selects the full vertex or
  edge set, same as omitting the argument.
  Previously an explicit NULL was coerced to an empty selection --
  an accident of as_igraph_vs(), never documented or tested.
- This fixes legacy positional recovery of diversity(),
  which died with a spurious "supplied more than once" error because
  re-evaluating the V(graph) default never compares identical().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
Defaults in migrated (`new`) signatures must be constant expressions:
literals, NULL/TRUE/FALSE/NA*/Inf/NaN, c()/list() of constants,
a unary sign, or the deprecated() sentinel.
The generator stops with an error on anything else --
option lookups, V(graph), cross-references, RNG draws --
and there is no escape hatch:
complex defaults are declared as NULL and resolved in the body.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
@krlmlr
krlmlr force-pushed the claude/migrations-const-defaults branch from 30194e8 to 915b5f2 Compare July 26, 2026 21:39
@krlmlr krlmlr changed the title feat(migrations): enforce constant defaults in migrated signatures feat(migrations): require constant defaults, resolve complex ones in the body Jul 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 915b5f2 is merged into main:

  • ✔️as_adjacency_matrix: 704ms -> 699ms [-2.01%, +0.66%]
  • ✔️as_biadjacency_matrix: 665ms -> 667ms [-0.51%, +1.28%]
  • ✔️as_data_frame_both: 1.27ms -> 1.27ms [-1.74%, +2.08%]
  • ✔️as_long_data_frame: 3.13ms -> 3.22ms [-0.15%, +5.58%]
  • ✔️es_attr_filter: 2.2ms -> 2.24ms [-0.72%, +4.44%]
  • ✔️graph_from_adjacency_matrix: 135ms -> 135ms [-1.25%, +0.86%]
  • ✔️graph_from_data_frame: 2.77ms -> 2.77ms [-2.01%, +1.59%]
  • ✔️vs_attr_filter: 1.3ms -> 1.29ms [-2.9%, +1.25%]
  • ❗🐌vs_by_name: 831µs -> 857µs [+0.21%, +6.08%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

claude and others added 5 commits July 27, 2026 18:12
Extends the constant-defaults treatment from the migrated functions to
every exported hand-written function:
106 defaults across 67 functions in 23 files
(games, layout, attributes, print, structural-properties, make,
embedding, operators, plot, and 14 more) are now declared as `NULL`
and resolved in the body after all arguments are available,
plus the two `plot_dendrogram()` S3 methods for coherence with their
generic.

Registry entries of the 32 affected migrated functions carry the same
`NULL` defaults, so the generated recovery blocks stay constant.

Deliberate exceptions:

- `layout_with_gem(temp.min = 1 / 10)` and `plot.igraph(mark.shape =
  1 / 2)` become the literal constants `0.1` and `0.5`.
- `layout_as_tree()`'s `root = numeric()` and `rootlevel = numeric()`
  stay: `is_constant_default()` now accepts zero-argument typed-empty
  constructors as literals.
- `print.igraph(max.lines = )` and the `normalize()` limits resolve via
  `missing()` instead of `is.null()`: NULL is a legal value there
  (print all lines; skip normalization along an axis) and keeps its
  meaning.
- `sample_motifs(cut.prob = NULL)` needs no body resolution: the C
  layer treats NULL as "no cuts", exactly like the old `rep(0, size)`,
  matching its `motifs()`/`count_motifs()` siblings.

Behavior notes:

- Passing NULL explicitly now means "use the default" for all treated
  arguments: selectors pick the full vertex/edge set (previously an
  accidental empty selection), option-backed arguments fall back to the
  igraph option (previously an empty combination spec or an error), and
  computed defaults are recomputed (previously usually an error).
- `layout_with_drl()`'s random default seed is now drawn inside the
  function, after `ensure_igraph()`; the RNG stream consumption of
  defaulted calls is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
`c()` evaluates to NULL, so the five empty-HRG fields that
`fit_hrg()` builds when no starting model is given (`left`, `right`,
`prob`, `edges`, `vertices` -- formerly `hrg()`'s signature defaults)
declared NULL values that meant "empty sequence".
NULL is now the resolve-in-body sentinel,
so empty sequences are spelled `numeric()` instead
(all five fields are `as.numeric()`-coerced on the spot),
and tests pass `integer()` where an empty id selection is meant.
`is_constant_default()` recognizes typed empties as literals,
pinned together with the behavior in test-constant-defaults.R:
an explicit typed empty keeps meaning "nothing selected",
while passing NULL to mean "empty" is no longer supported for treated
arguments -- a deliberate breaking change
(`max_degree(g, v = NULL)` now means all vertices).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4783887 is merged into main:

  • ✔️as_adjacency_matrix: 848ms -> 851ms [-1.71%, +2.47%]
  • ✔️as_biadjacency_matrix: 797ms -> 804ms [-0.84%, +2.78%]
  • ✔️as_data_frame_both: 1.56ms -> 1.55ms [-1.19%, +0.64%]
  • ❗🐌as_long_data_frame: 3.95ms -> 3.99ms [+0.34%, +1.87%]
  • ✔️es_attr_filter: 3.1ms -> 3.13ms [-1.1%, +2.66%]
  • ✔️graph_from_adjacency_matrix: 150ms -> 152ms [-1.05%, +3.99%]
  • ✔️graph_from_data_frame: 3.59ms -> 3.61ms [-0.86%, +1.98%]
  • ✔️vs_attr_filter: 1.75ms -> 1.75ms [-3.27%, +2.9%]
  • ✔️vs_by_name: 1.18ms -> 1.18ms [-1.64%, +2.29%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants